Skip to content

Conversation

@DarkAtra
Copy link
Contributor

@DarkAtra DarkAtra commented Sep 24, 2025

Fixes #1162

Summary by CodeRabbit

  • Bug Fixes

    • ID handling and serialization adjusted to improve equality/hash behavior; may require minor integration updates.
  • Tests

    • Added a regression test to verify collections load from a legacy 4.3.0 database sample (duplicate test entry present).
  • Chores

    • Updated ignore rules to allow tracking the no2-v4.3.0.db sample file.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 24, 2025

Walkthrough

Adds a negated .gitignore entry to track no2-v4.3.0.db, introduces a new JUnit test method testIssue1162 (duplicated) that copies and opens a bundled 4.3.0 DB via MVStoreModule and asserts collection contents, and renames NitriteId's internal field idValue_idValue and adds a public getter getIdValue() with all internal references updated.

Changes

Cohort / File(s) Summary of Changes
Repo config
./.gitignore
Added negated ignore rule !no2-v4.3.0.db so that no2-v4.3.0.db is tracked.
Tests: MVStore adapter
nitrite-mvstore-adapter/src/test/java/org/dizitart/no2/NitriteTest.java
Added import java.nio.file.StandardCopyOption; added a public test method testIssue1162() (appears twice / duplicated) that copies the bundled no2-v4.3.0.db to a temp path, opens it with MVStoreModule, and asserts collection size and first document ID; minor formatting change in an anonymous inner class closing brace.
Core: NitriteId internals
nitrite/src/main/java/org/dizitart/no2/collection/NitriteId.java
Renamed field private long idValueprivate long _idValue; added public getter public long getIdValue(); updated all internal references (constructors, compareTo, toString, writeObject/readObject) to use _idValue. No public method signatures removed.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Test as JUnit Test
  participant FS as FileSystem (temp)
  participant MV as MVStoreModule
  participant DB as Nitrite DB
  participant Coll as Collection
  Note right of Test: testIssue1162 flow (loads bundled v4.3.0 DB)
  Test->>FS: copy packaged `no2-v4.3.0.db` to temp
  Test->>MV: configure MVStoreModule with temp DB path
  Test->>DB: open Nitrite instance (via MVStoreModule)
  DB->>Coll: load collection data (deserialize stored objects including NitriteId)
  Coll-->>Test: return collection size and documents
  alt deserialization compatible
    Test-->>Test: assert expected size and first document ID
  else incompatible (InvalidClassException)
    DB-->>Test: exception thrown during deserialization
  end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

I hop a patch from old to new,
I copy a file and peek inside too.
A tiny name gets an underscore cheer,
Tests probe the past to keep it near.
Thump — I guard the bytes, nibble a bug clear. 🥕🐇

Pre-merge checks and finishing touches

❌ Failed checks (3 warnings)
Check name Status Explanation Resolution
Linked Issues Check ⚠️ Warning The patch as summarized does not demonstrate that it meets the linked-issue objectives: the private field was renamed from idValue to _idValue and writeObject/readObject were updated to use the new name, which does not preserve the original serialized field identity and will still trigger java.io.InvalidClassException when reading databases created with 4.3.0; the added tests and import changes do not show handling of the legacy "idValue" field. Because legacy serialized data must be read using the original field name (or explicitly mapped), this change as-is is unlikely to restore compatibility or prevent rollback/data loss. A correct resolution must explicitly accept the legacy field name (for example by retaining a compatible idValue field or using ObjectInputStream.GetField in readObject to map idValue into the new field) and include a regression test that opens a 4.3.0 database. Rework serialization handling to accept legacy serialized data by preserving the original field name or implementing readObject logic that falls back to "idValue" when present, add the reproducer-based regression test to validate opening a 4.3.0 database, and keep unrelated changes in separate commits.
Out of Scope Changes Check ⚠️ Warning The changes include at least one repository-focused edit (a negated .gitignore entry) and the raw summary indicates duplicate test method additions, both of which are outside the core objective of restoring NitriteId serialization compatibility; the test additions that validate the fix are in-scope but repository ignore tweaks and accidental duplicates are not. Keeping unrelated repository/config changes or accidental duplicates in this PR reduces focus and makes review harder. Remove or move the .gitignore change to a separate commit/PR and clean up duplicate or accidental test method additions so this PR contains only the serialization fix and its focused regression test.
Docstring Coverage ⚠️ Warning Docstring coverage is 7.69% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title is concise, single-sentence, and accurately summarizes the primary intent of the changeset—restoring NitriteId serialization compatibility to address gh-1162—so it clearly communicates the main change to reviewers.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 251595a and 338595e.

⛔ Files ignored due to path filters (1)
  • nitrite-mvstore-adapter/src/test/resources/no2-v4.3.0.db is excluded by !**/*.db
📒 Files selected for processing (3)
  • .gitignore (1 hunks)
  • nitrite-mvstore-adapter/src/test/java/org/dizitart/no2/NitriteTest.java (3 hunks)
  • nitrite/src/main/java/org/dizitart/no2/collection/NitriteId.java (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • .gitignore
  • nitrite/src/main/java/org/dizitart/no2/collection/NitriteId.java
🧰 Additional context used
🧬 Code graph analysis (1)
nitrite-mvstore-adapter/src/test/java/org/dizitart/no2/NitriteTest.java (1)
nitrite-mvstore-adapter/src/main/java/org/dizitart/no2/mvstore/MVStoreModule.java (1)
  • MVStoreModule (36-77)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
  • GitHub Check: Verify GraalVM 17 compatibility on macos-latest
  • GitHub Check: Verify GraalVM 21 compatibility on macos-latest
  • GitHub Check: Verify GraalVM 17 compatibility on windows-latest
  • GitHub Check: Verify GraalVM 17 compatibility on ubuntu-latest
  • GitHub Check: Verify GraalVM 21 compatibility on windows-latest
  • GitHub Check: Build with Java 11 in Ubuntu
  • GitHub Check: Build with Java 17 in Windows
  • GitHub Check: Build with Java 11 in Windows
  • GitHub Check: Verify GraalVM 21 compatibility on ubuntu-latest
  • GitHub Check: Build with Java 17 in Ubuntu
  • GitHub Check: Build with Java 17 in MacOS
  • GitHub Check: Codacy Static Code Analysis
  • GitHub Check: Analyze (java)
🔇 Additional comments (4)
nitrite-mvstore-adapter/src/test/java/org/dizitart/no2/NitriteTest.java (4)

56-56: Import addition looks good for file copying.

The added import java.nio.file.StandardCopyOption is needed for the new test method that copies a bundled database file using Files.copy() with StandardCopyOption.REPLACE_EXISTING.


626-626: Minor formatting change - anonymous inner class closing.

The formatting change from }; to } for the anonymous inner class is a minor style adjustment that doesn't affect functionality.


708-708: Confirm NitriteId.getIdValue() method availability.
Public getter found: nitrite/src/main/java/org/dizitart/no2/collection/NitriteId.java (lines 121–123) — public long getIdValue() { return _idValue; }.


686-710: NitriteId serialization compatibility verified — approve.

NitriteId now uses private _idValue with getIdValue() and implements writeObject/readObject; mvstore compat maps legacy org.dizitart.no2.NitriteId → Compat.NitriteId and UpgradeUtil converts it, and there is a test that opens the bundled v4.3.0 DB and asserts the id.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@DarkAtra DarkAtra force-pushed the gh-1162/ensure-NitriteId-serialization-is-compatible-with-previous-versions branch from ce59500 to 36df63a Compare September 24, 2025 12:55
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
nitrite/src/main/java/org/dizitart/no2/collection/NitriteId.java (1)

49-62: Good fix for backward compatibility; consider hardening against accidental default serialization

Renaming the field and adding the explicit warning is helpful. Given custom writeObject/readObject are now authoritative, consider marking the field transient to prevent accidental default serialization if someone later modifies writeObject. It’s safe (we set it explicitly in readObject/ctors).

-    private long _idValue;
+    private transient long _idValue;
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ce59500 and 36df63a.

⛔ Files ignored due to path filters (1)
  • nitrite-mvstore-adapter/src/test/resources/no2-v4.3.0.db is excluded by !**/*.db
📒 Files selected for processing (3)
  • .gitignore (1 hunks)
  • nitrite-mvstore-adapter/src/test/java/org/dizitart/no2/NitriteTest.java (3 hunks)
  • nitrite/src/main/java/org/dizitart/no2/collection/NitriteId.java (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • .gitignore
  • nitrite-mvstore-adapter/src/test/java/org/dizitart/no2/NitriteTest.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
  • GitHub Check: Codacy Static Code Analysis
  • GitHub Check: Verify GraalVM 21 compatibility on macos-latest
  • GitHub Check: Verify GraalVM 21 compatibility on windows-latest
  • GitHub Check: Verify GraalVM 17 compatibility on windows-latest
  • GitHub Check: Build with Java 17 in Windows
  • GitHub Check: Build with Java 11 in MacOS
  • GitHub Check: Build with Java 17 in MacOS
  • GitHub Check: Build with Java 11 in Windows
  • GitHub Check: Analyze (java)
  • GitHub Check: Build with Java 17 in Ubuntu
  • GitHub Check: Build with Java 11 in Ubuntu
🔇 Additional comments (4)
nitrite/src/main/java/org/dizitart/no2/collection/NitriteId.java (4)

117-120: Explicit accessor restores the prior Lombok-generated API

Matches the previous getIdValue contract. Looks good.


123-123: compareTo uses the underlying value directly — OK

Comparison is consistent with equals/hashCode.


128-129: toString remains stable — OK

Preserves the wire/text form ID_PREFIX + value + ID_SUFFIX.


132-137: Widen NitriteId.readObject signature to include ClassNotFoundException and preserve UTF serialization

nitrite/src/main/java/org/dizitart/no2/collection/NitriteId.java uses stream.writeUTF/stream.readUTF; keep that encoding and change readObject to the standard signature:

-    private void readObject(ObjectInputStream stream) throws IOException {
+    private void readObject(ObjectInputStream stream) throws IOException, ClassNotFoundException {
         _idValue = Long.parseLong(stream.readUTF());
     }

Could not find tag v4.3.0 in this clone to confirm historical parity — verify that v4.3.0 used writeUTF/readUTF before merging.

@DarkAtra DarkAtra force-pushed the gh-1162/ensure-NitriteId-serialization-is-compatible-with-previous-versions branch 2 times, most recently from 6a39d0d to 251595a Compare September 24, 2025 14:15
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
nitrite/src/main/java/org/dizitart/no2/collection/NitriteId.java (2)

123-129: Optional: add identity fast-path in equals.

Micro-optimization; no behavior change.

     @Override
     public boolean equals(Object o) {
+        if (this == o) {
+            return true;
+        }
         if (!(o instanceof NitriteId)) {
             return false;
         }
         return idValue == ((NitriteId) o).idValue;
     }

27-27: Avoid boxing in hashCode and drop the unused Objects import.

Use Long.hashCode to prevent boxing; then the Objects import can be removed.

-import java.util.Objects;
     @Override
     public int hashCode() {
-        return Objects.hashCode(idValue);
+        return Long.hashCode(idValue);
     }

Also applies to: 131-134

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6a39d0d and 251595a.

⛔ Files ignored due to path filters (1)
  • nitrite-mvstore-adapter/src/test/resources/no2-v4.3.0.db is excluded by !**/*.db
📒 Files selected for processing (3)
  • .gitignore (1 hunks)
  • nitrite-mvstore-adapter/src/test/java/org/dizitart/no2/NitriteTest.java (3 hunks)
  • nitrite/src/main/java/org/dizitart/no2/collection/NitriteId.java (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • nitrite-mvstore-adapter/src/test/java/org/dizitart/no2/NitriteTest.java
  • .gitignore
🧰 Additional context used
🧬 Code graph analysis (1)
nitrite/src/main/java/org/dizitart/no2/collection/NitriteId.java (2)
nitrite/src/main/java/org/dizitart/no2/exceptions/InvalidIdException.java (1)
  • InvalidIdException (28-47)
nitrite-jackson-mapper/src/test/java/org/dizitart/no2/test/RepositoryJoinTest.java (1)
  • Data (216-222)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
  • GitHub Check: Codacy Static Code Analysis
  • GitHub Check: Verify GraalVM 17 compatibility on ubuntu-latest
  • GitHub Check: Analyze (java)
  • GitHub Check: Verify GraalVM 21 compatibility on windows-latest
  • GitHub Check: Verify GraalVM 21 compatibility on macos-latest
  • GitHub Check: Verify GraalVM 17 compatibility on macos-latest
  • GitHub Check: Build with Java 11 in Ubuntu
  • GitHub Check: Verify GraalVM 17 compatibility on windows-latest
  • GitHub Check: Build with Java 17 in Windows
  • GitHub Check: Verify GraalVM 21 compatibility on ubuntu-latest
  • GitHub Check: Build with Java 11 in Windows
  • GitHub Check: Build with Java 17 in MacOS
  • GitHub Check: Build with Java 17 in Ubuntu
  • GitHub Check: Build with Java 11 in MacOS
🔇 Additional comments (1)
nitrite/src/main/java/org/dizitart/no2/collection/NitriteId.java (1)

51-51: Marking idValue transient is the right compatibility fix.

This aligns with custom writeObject/readObject and should resolve the InvalidClassException reported in gh-1162.

Please confirm this passes the linked reproducer and the new 4.3.0 DB test case on your side.

@DarkAtra DarkAtra force-pushed the gh-1162/ensure-NitriteId-serialization-is-compatible-with-previous-versions branch from 251595a to 338595e Compare September 24, 2025 14:39
@anidotnet anidotnet merged commit 352af93 into nitrite:main Sep 25, 2025
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: Updating to 4.3.1 causes existing databases to no open correctly

2 participants